t2m: stabilized twist transport in the direction retarget (#857)#886
t2m: stabilized twist transport in the direction retarget (#857)#886fernandotonon wants to merge 2 commits into
Conversation
The bind-referenced direction retarget aimed each bone but inherited the target bind's roll — gesture clips read flat and the roll component between mismatched decomposition poles leaked into swing (arm amplitude +38% on Mixamo self-retarget). Now the source's swing/twist split is recomposed about the SAME pole on the target: Qbase = arc(dt_bind -> d_ref) * Wt_bind (once per bone) Wt(f) = twist(theta*gain, ds) * arc(d_ref -> ds) * Qbase theta is the source's signed roll about its reference bone direction, wrapped to (-pi,pi] and unwrapped across frames (a +-180deg pop would flip the roll once a gain != 1 scales it), capped at 150deg; collars are damped 0.5x (they share the shoulder line's roll). Direction tracking stays absolute, so cross-rig robustness is unchanged. Mixamo Walk self-parity: 2.22deg -> 0.03deg mean joint delta (target was <=1.6deg); spine amplitude restored (abdomen 2.3deg -> 9.0deg amp), legs exact; Rumba walk/dance/wave and Quaternius Knight renders upright with natural hanging arms. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe bind-referenced motion retargeting path now decomposes, unwraps, caps, role-scales, and reapplies source bone twist. New tests cover arm roll transport and continuous damped collar twist. ChangesTwist transport retargeting
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant SourceFrame
participant AnimationMerger
participant TargetAnimation
SourceFrame->>AnimationMerger: Supply frame rotation and bone axis
AnimationMerger->>AnimationMerger: Decompose, unwrap, cap, and scale twist
AnimationMerger->>TargetAnimation: Apply aimed rotation with transported twist
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98fa2c54ef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| has[static_cast<size_t>(c)] = 1; | ||
| twistTheta[static_cast<size_t>(f)] | ||
| [static_cast<size_t>(c)] = | ||
| std::clamp(th, -kTwistCap, kTwistCap); |
There was a problem hiding this comment.
For clips where a damped role unwraps past 150° (the added collar test drives role 10 to 240°), this stores only 150° in twistTheta; when the value is later multiplied by kTwistGain[c] (0.5 for collars), the final roll becomes 75° instead of the intended/tested 120°. This silently attenuates large unwrapped collar rolls even though the gain is supposed to damp the transported angle rather than reduce the cap first.
Useful? React with 👍 / 👎.
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/AnimationMerger_test.cpp (1)
1236-1306: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: created skeleton/mesh resources leak, and
entis not destroyed on earlyASSERT.The skeleton
"twist_collar_skel"and mesh"twist_collar_mesh"are created but never removed, andsm->destroyEntity(ent)at Line 1306 is skipped ifASSERT_TRUE(res.ok)(Line 1279) fails. On a re-run within the same process this can surface as "resource already exists" or leaked scene nodes. Consider unloading the resources in a scope guard /TearDownand destroying the entity before the assertions can bail.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/AnimationMerger_test.cpp` around lines 1236 - 1306, Ensure the test’s temporary Entity and resources are cleaned up on every exit path, including failure of ASSERT_TRUE(res.ok). Add scope-based cleanup around the created ent, skelRes, and mesh resources, destroying the entity before removing the "twist_collar_skel" and "twist_collar_mesh" resources; retain the existing assertions and sampling behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/AnimationMerger_test.cpp`:
- Around line 1236-1306: Ensure the test’s temporary Entity and resources are
cleaned up on every exit path, including failure of ASSERT_TRUE(res.ok). Add
scope-based cleanup around the created ent, skelRes, and mesh resources,
destroying the entity before removing the "twist_collar_skel" and
"twist_collar_mesh" resources; retain the existing assertions and sampling
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 39f1c543-021b-4e63-81fd-3bf0a2d17053
📒 Files selected for processing (2)
src/AnimationMerger.cppsrc/AnimationMerger_test.cpp
…ectation (#857) The arm-rig helper resolves only 9/22 roles, below applyMotionClip's >=11 humanoid gate — the roll test now builds a 13-role rig (+hands, +feet). And the 150-degree runaway-unwrap cap applies BEFORE the collar gain, so a 240-degree source roll lands at 150 x 0.5 = 75 degrees, not 120; the continuity assertion (the actual unwrap regression check) is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/AnimationMerger_test.cpp (2)
1290-1291: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd
ASSERT_NE(ent, nullptr)before usingent.The first test guards entity creation at Line 1219, but this test dereferences
ent->getSkeleton()immediately. A null return fromcreateEntitywould segfault instead of producing a clean test failure.🛡️ Proposed fix
Ogre::Entity* ent = sm->createEntity("twist_collar_ent", mesh); + ASSERT_NE(ent, nullptr); Ogre::SkeletonInstance* skel = ent->getSkeleton();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/AnimationMerger_test.cpp` around lines 1290 - 1291, Add an ASSERT_NE check confirming ent is not nullptr immediately after createEntity in this test and before calling ent->getSkeleton(), so entity creation failures produce a test assertion instead of dereferencing a null pointer.
1311-1312: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate stale comment to reflect the 150° cap.
The comment says "end near 240° × 0.5 = 120°" but the assertion at Line 1336 and the comment at Lines 1334-1336 correctly expect 75° (150° cap × 0.5 gain). This contradiction could mislead debugging.
📝 Proposed fix
- // Sample densely: the collar's world orientation must move CONTINUOUSLY - // (no wrap snap) and end near 240° × 0.5 = 120° about +X. + // Sample densely: the collar's world orientation must move CONTINUOUSLY + // (no wrap snap) and end near 150° (cap) × 0.5 = 75° about +X.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/AnimationMerger_test.cpp` around lines 1311 - 1312, Update the stale orientation comment near the dense-sampling test to describe the 150° cap and 0.5 gain, with an expected end near 75° about +X; leave the existing assertion and other correct comments unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/AnimationMerger_test.cpp`:
- Around line 1290-1291: Add an ASSERT_NE check confirming ent is not nullptr
immediately after createEntity in this test and before calling
ent->getSkeleton(), so entity creation failures produce a test assertion instead
of dereferencing a null pointer.
- Around line 1311-1312: Update the stale orientation comment near the
dense-sampling test to describe the 150° cap and 0.5 gain, with an expected end
near 75° about +X; leave the existing assertion and other correct comments
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 42447cb0-ee61-487b-a281-6a033b0a0fdb
📒 Files selected for processing (1)
src/AnimationMerger_test.cpp
|
|
Superseded by the combined PR #904 (all three stacked slices merged into one branch, |
1 participant
Closes #857 (epic #837).
The bind-referenced direction retarget (PR #843) aimed each bone's direction but deliberately dropped twist (roll about the bone axis) — gesture-heavy clips lost forearm/spine roll and read flat, at a measured ~0.6° self-parity penalty. This PR transports it, stabilized.
What changed
Per role, the source's frame rotation is decomposed into swing (the direction, already transported) + twist about the bone axis:
θ(f)= signed angle ofarc(d_ref → d(f))⁻¹ · Δ(f)about the reference bone direction, wrapped to (−π,π], unwrapped across frames (a ±180° pop near the shortest-arc degeneracy would flip the roll mid-clip once a gain ≠ 1 scales it), and capped at 150° as a runaway-unwrap guard.The key correctness insight (found via the harness, not in the issue): the twist must be recomposed about the same pole the source decomposed about. Recomposing per-frame from the target bind direction (the issue's original formula) decomposes about a different pole than the source — the roll component between the two poles leaks into swing and inflates amplitude (measured: Mixamo self-retarget arm amplitude +38%, elbow error 3.3°→6.3°). So the target now anchors its roll baseline once at the source's reference direction:
With matched poles the per-frame relative motion is exactly the source's world delta conjugated into target axes — direction tracking stays absolute (cross-rig robustness unchanged), roll transports losslessly. Per-role gains: collars damped 0.5× (they share the shoulder line's roll), everything else full.
Harness results (acceptance)
Controlled A/B (same source clip pinned — the library take-pick is quality-weighted random, so uncontrolled A/Bs are noisy): Knight silhouettes identical between pre-#857, pole-change-only, and full twist; Rumba walk/dance/wave upright, dance now carries the source's salsa turn (hip twist transports facing), wave unchanged.
Tests
TwistTransportCarriesBoneRoll— pure roll on the source (direction constant, non-identity source rest exercising the restWorld conjugation) reproduces on the target bone; direction invariant; legs untouched.TwistUnwrapKeepsDampedCollarContinuous— 0→240° collar roll across the ±180° wrap stays continuous under the 0.5× gain and lands at 120°.(Ogre-dependent — run on Linux CI under Xvfb, like the Retarget post-processing controls: Mixamo-style arm-space slider #854 suite.)
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests